Optimize mapObj by lencioni · Pull Request #202 · Khan/aphrodite

lencioni · 2017-03-04T18:06:25Z

I did some profiling of StyleSheet.create and noticed that mapObj was a
good target for optimization.

Methodology

I created an HTML document with the following code in it:

<script type="text/javascript" src="./dist/aphrodite.umd.js"></script>
<!-- setup -->
<script type="text/javascript">
// build up an array of styles objects to run our test on
var styles = [];
for (var i = 0; i < 10000; i += 1) {
  styles.push({
    [`a${Math.random()}`]: {
      [`a${Math.random()}`]: Math.random(),
      [`b${Math.random()}`]: String(Math.random()),
      [`c${Math.random()}`]: String(Math.random()),
    },

    [`b${Math.random()}`]: {
      [`a${Math.random()}`]: Math.random(),
      [`b${Math.random()}`]: String(Math.random()),
      [`c${Math.random()}`]: String(Math.random()),
    },

    [`c${Math.random()}`]: {
      [`a${Math.random()}`]: Math.random(),
      [`b${Math.random()}`]: String(Math.random()),
      [`c${Math.random()}`]: String(Math.random()),
    },
  });
}
</script>

<!-- test -->
<script type="text/javascript">
setTimeout(() => {
  performance.mark('start_run');
  for (var i = 0; i < styles.length; i += 1) {
    // prevent caching optimizations
    eval('');
    performance.mark('start_stylesheet_create');
    aphrodite.StyleSheet.create(styles[i]);
    performance.mark('end_stylesheet_create');
    performance.measure(
      'aphrodite.StyleSheet.create',
      'start_stylesheet_create',
      'end_stylesheet_create'
    );
    performance.clearMarks('start_stylesheet_create', 'end_stylesheet_create');
  }
  performance.mark('end_run');
  performance.measure(`Benchmark ${styles.length}`, 'start_run', 'end_run');
  performance.clearMarks();
});
</script>

Then, looking at the timeline tool in Chrome, I loaded the page a few
times before and after this change. Similarly, I ran a CPU profile
before and after this change through 5 page reloads each.

In this test, the timeline was not very helpful, I think because of the
testing overhead. However, the CPU profile was very clear. Before this
change, normalizing for the callback showing up in a different part of
the profile, mapObj took ~317ms and after this change, it drops to
~211ms. StyleSheet.create drops from ~755ms to ~670ms or roughly 11%
faster. The rest of the time in StyleSheet.create is spent in
murmurhash2_32_gc and hashObject.

cc @ljharb

In profiling StyleSheet.create, I noticed that much of the time was spent hashing. So, I found a faster hashing algorithm. The implementation was taken from: https://github.com/darkskyapp/string-hash According to this StackExchange post, this algorithm doesn't have as good of randomness, but it has about the same percentage of collisions. I don't think randomness matters for this application, so I think this is okay. Using similar methodology to Khan#202, this appears to make StylSheet.create ~15% faster (~220ms to ~185ms).

In profiling StyleSheet.create, I noticed that much of the time was spent hashing. So, I found a faster hashing algorithm. The implementation was taken from: https://github.com/darkskyapp/string-hash According to this StackExchange post, this algorithm doesn't have as good of randomness, but it has about the same percentage of collisions. I don't think randomness matters for this application, so I think this is okay. http://softwareengineering.stackexchange.com/a/145633 Using similar methodology to Khan#202, this appears to make StylSheet.create ~15% faster (~220ms to ~185ms).

ljharb · 2017-03-04T22:02:51Z

+  const mappedObj = {};
+  for (let i = 0; i < keys.length; i += 1) {
+    const key = keys[i];
+    const [newKey, newValue] = fn.call(undefined, [key, obj[key]]);


i'd think that fn([key, obj[key]]) would be much faster than using .call?

I thought so too, but this seemed to be a bit better, at least in Chrome. I was thinking it might have to do with thisObj being undefined, but maybe I'll take another look at the perf here.

They seem to be about the same. I'll change it to fn() because it is less weird than fn.call.

I thought maybe it would be faster by removing the array here and passing args directly, but that didn't seem to really make a difference.

ljharb · 2017-03-04T22:03:22Z

+  const keys = Object.keys(obj);
+  const mappedObj = {};
+  for (let i = 0; i < keys.length; i += 1) {
+    const key = keys[i];


if you're looking for performance, avoiding block-scoped vars in loops is a good way to do it - ie, repeating the keys[i] in two places probably will give you a slight bump

Yeah, maybe I'll give it a go. I originally avoided it for readability, and I think the performance improvement will be nominal. But maybe it will help with GC.

In profiling StyleSheet.create, I noticed that much of the time was spent hashing. So, I found a faster hashing algorithm. The implementation was taken from: https://github.com/darkskyapp/string-hash According to this StackExchange post, this algorithm doesn't have as good of randomness, but it has about the same percentage of collisions. I don't think randomness matters for this application, so I think this is okay. http://softwareengineering.stackexchange.com/a/145633 Using similar methodology to Khan#202, this appears to make StylSheet.create ~15% faster (~220ms to ~185ms).

lencioni · 2017-03-05T15:49:17Z

JSPerf: https://jsperf.com/aphrodite-mapobj-refactor/1

I'm seeing the new version about 10% faster in Chrome, 13% faster in Firefox.

xymostech

LGTM! Some small nit. Thanks for doing such thorough performance testing!

xymostech · 2017-03-06T21:51:42Z

+    const [newKey, newValue] = fn([keys[i], obj[keys[i]]]);
+    mappedObj[newKey] = newValue;
+  }
+  return mappedObj;


nit: we use 4 space indents

I did some profiling of StyleSheet.create and noticed that mapObj was a good target for optimization. Methodology I created an HTML document with the following code in it: ```html <script type="text/javascript" src="./dist/aphrodite.umd.js"></script>  <script type="text/javascript"> // build up an array of styles objects to run our test on var styles = []; for (var i = 0; i < 10000; i += 1) { styles.push({ [`a${Math.random()}`]: { [`a${Math.random()}`]: Math.random(), [`b${Math.random()}`]: String(Math.random()), [`c${Math.random()}`]: String(Math.random()), }, [`b${Math.random()}`]: { [`a${Math.random()}`]: Math.random(), [`b${Math.random()}`]: String(Math.random()), [`c${Math.random()}`]: String(Math.random()), }, [`c${Math.random()}`]: { [`a${Math.random()}`]: Math.random(), [`b${Math.random()}`]: String(Math.random()), [`c${Math.random()}`]: String(Math.random()), }, }); } </script>  <script type="text/javascript"> setTimeout(() => { performance.mark('start_run'); for (var i = 0; i < styles.length; i += 1) { // prevent caching optimizations eval(''); performance.mark('start_stylesheet_create'); aphrodite.StyleSheet.create(styles[i]); performance.mark('end_stylesheet_create'); performance.measure( 'aphrodite.StyleSheet.create', 'start_stylesheet_create', 'end_stylesheet_create' ); performance.clearMarks('start_stylesheet_create', 'end_stylesheet_create'); } performance.mark('end_run'); performance.measure(`Benchmark ${styles.length}`, 'start_run', 'end_run'); performance.clearMarks(); }); </script> ``` Then, looking at the timeline tool in Chrome, I loaded the page a few times before and after this change. Similarly, I ran a CPU profile before and after this change through 5 page reloads each. In this test, the timeline was not very helpful, I think because of the testing overhead. However, the CPU profile was very clear. Before this change, normalizing for the callback showing up in a different part of the profile, `mapObj` took ~317ms and after this change, it drops to ~211ms. `StyleSheet.create` drops from ~755ms to ~670ms or roughly 11% faster. The rest of the time in `StyleSheet.create` is spent in `murmurhash2_32_gc` and `hashObject`.

xymostech

LGTM now!

In profiling StyleSheet.create, I noticed that much of the time was spent hashing. So, I found a faster hashing algorithm. The implementation was taken from: https://github.com/darkskyapp/string-hash According to this StackExchange post, this algorithm doesn't have as good of randomness, but it has about the same percentage of collisions. I don't think randomness matters for this application, so I think this is okay. http://softwareengineering.stackexchange.com/a/145633 Using similar methodology to Khan#202, this appears to make StylSheet.create ~15% faster (~220ms to ~185ms).

* Replace murmur hash with djb2 hash In profiling StyleSheet.create, I noticed that much of the time was spent hashing. So, I found a faster hashing algorithm. The implementation was taken from: https://github.com/darkskyapp/string-hash According to this StackExchange post, this algorithm doesn't have as good of randomness, but it has about the same percentage of collisions. I don't think randomness matters for this application, so I think this is okay. http://softwareengineering.stackexchange.com/a/145633 Using similar methodology to #202, this appears to make StylSheet.create ~15% faster (~220ms to ~185ms). * Depend on string-hash for djb2 hashing algorithm This is where I copied the code for this algorithm from, seems like we might as well just bring in the dependency for it.

lencioni mentioned this pull request Mar 4, 2017

Replace murmur hash with djb2 hash #203

Merged

ljharb reviewed Mar 4, 2017

View reviewed changes

lencioni force-pushed the mapObj branch from c286dbb to 7ea7254 Compare March 4, 2017 22:48

xymostech suggested changes Mar 6, 2017

View reviewed changes

lencioni force-pushed the mapObj branch from 7ea7254 to 686dc02 Compare March 6, 2017 21:59

xymostech approved these changes Mar 7, 2017

View reviewed changes

xymostech merged commit 5e9a0cb into Khan:master Mar 7, 2017

lencioni deleted the mapObj branch July 20, 2017 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize mapObj#202

Optimize mapObj#202
xymostech merged 1 commit into
Khan:masterfrom
lencioni:mapObj

lencioni commented Mar 4, 2017

Uh oh!

ljharb Mar 4, 2017

Uh oh!

lencioni Mar 4, 2017

Uh oh!

lencioni Mar 4, 2017

Uh oh!

ljharb Mar 4, 2017

Uh oh!

lencioni Mar 4, 2017

Uh oh!

lencioni commented Mar 5, 2017

Uh oh!

xymostech left a comment

Uh oh!

xymostech Mar 6, 2017

Uh oh!

lencioni Mar 6, 2017

Uh oh!

xymostech left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lencioni commented Mar 4, 2017

Uh oh!

ljharb Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

lencioni Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

lencioni Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

ljharb Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

lencioni Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

lencioni commented Mar 5, 2017

Uh oh!

xymostech left a comment

Choose a reason for hiding this comment

Uh oh!

xymostech Mar 6, 2017

Choose a reason for hiding this comment

Uh oh!

lencioni Mar 6, 2017

Choose a reason for hiding this comment

Uh oh!

xymostech left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants